Update `impl`-image, tests and enable tool version strings #4

se-bi · 2020-12-12T23:55:13Z

Thanks, @eine for the discussion on #1 and adding the impl image.
(Surprisingly, the documentation was ahead ... 😌 )

Some ideas came up on that:

Separating impl images regarding ice40 and ecp5 architectures
Adding tools of icestorm and prjtrellis, respectively
Clone Github Projects instead of Tar-archive via codeload, to have a meaningful --version return of the builds
Add some test calls of tools (--version or similar)

(see the CI runs, which are not running unchanged parts and pushing into different DockerHub Repositories
se-bi/hdl-containers@develop...se-bi:test/build-version)

eine

Hi @se-bi! Sorry for the delay in getting back to you.

Separating impl images regarding ice40 and ecp5 architectures

Adding tools of icestorm and prjtrellis, respectively

Agree to both changes, but not in the same images. That is:

impl:ice40: GHDL + Yosys + nextpnr-ice40
impl:ecp5: GHDL + Yosys + nextpnr-ecp5
impl:pnr: GHDL + Yosys + nextpnr-ice40 + nextpnr-ecp5
impl:icestorm: impl:ice40 + icestorm
impl:prjtrellis: impl:ecp5 + prjtrellis
impl:latest: impl:pnr + icestorm + prjtrellis

Feel free to implement some of them only (the ones you are most interested in). Also, feel free to discuss the naming.

Clone Github Projects instead of Tar-archive via codeload, to have a meaningful --version return of the builds

While it might be acceptable in build images/stages, it's a no-go for the "production" images. Be careful with that.

Add some test calls of tools (--version or similar)

This is nice! However, checks such as --version or which should be better contributed to hdl/smoke-tests. smoke-tests is a submodule of this repo, located in test/smoke-tests. Test scripts in this repo should first call the relevant smoke-tests and then run further checks.

.github/workflows/impl.yml

ghdl-yosys-plugin.dockerfile

impl.dockerfile

eine · 2021-01-03T06:13:12Z

nextpnr.dockerfile

- && cd /tmp/nextpnr \
- && curl -fsSL https://codeload.github.com/YosysHQ/nextpnr/tar.gz/master | tar xzf - --strip-components=1 \
- && cd build \
+RUN git clone --depth 1 https://github.com/YosysHQ/nextpnr.git /tmp/nextpnr \


With --depth 1 tags are not retrieved, are they? Some projects show the version as a difference from the last tag, plug the SHA of the current commit. See, for example GHDL 1.0-dev (v0.37.0-1123-g8ed35277) [Dunoon edition]. "Shallow clones" don't work in those cases.

eine · 2021-01-03T06:16:59Z

nextpnr.dockerfile

- && cd /tmp/nextpnr \
- && curl -fsSL https://codeload.github.com/YosysHQ/nextpnr/tar.gz/master | tar xzf - --strip-components=1 \
- && cd build \
+RUN git clone --depth 1 https://github.com/YosysHQ/nextpnr.git /tmp/nextpnr \


There is an experimental feature that allows using resources from previous stages without increasing the size of the current stage. See:

https://github.com/ghdl/docker/blob/master/vunit.dockerfile#L35

https://github.com/ghdl/docker/blob/master/vunit.dockerfile#L1

That would allow you to clone the repo once only.

Anyway, this is not critical.

What about splitting the stages here, and base each build on that same layer, where the git is fetched once?

Yes, that makes sense. Cloning the repo twice was an overlook!

At the same time, build-all should be build-generic. There is no need for building nextpnr-ice40 and nextpnr-ecp5 twice! See #10 (comment).

However, if you want to contribute that, better do it in a PR for that only. That will allow us to merge it faster, and #10 might be rebased on top of it.

This:

At the same time, build-all should be build-generic
and
See #10 (comment).

resolves some confusion with building some parts of nextpnr twice
and I will integrate that.

eine · 2021-01-15T03:48:34Z

@se-bi, I suggest squashing your commits in a single one, and then rebasing that on top of master. I believe that will make it easier to deal with possible conflicts. You can later split them again, if you want to keep several commits in the history. For instance, the change related to adding git in hdlc/build:build might be handled as a separated PR. That would make this one easier to handle.

se-bi · 2021-01-17T00:04:04Z

Hey @eine, sorry for the delay.

@se-bi, I suggest squashing your commits in a single one, and then rebasing that on top of master. I believe that will make it easier to deal with possible conflicts. You can later split them again, if you want to keep several commits in the history.

I rebased everything and squashed some commits.

For instance, the change related to adding git in hdlc/build:build might be handled as a separated PR. That would make this one easier to handle.

Should we also make the changes within nextpnr which are the base for some in impl also in a separate PR?
Since the dockerBuild of impl would fail without the necessary images and no dependencies of the workflows.

(I tried integrating that dependency once, but didn't work as soon as expected and was not patient enough...)

Btw. thanks @umarcor for the scripts in .github/bin, they ease life with a fork 🙈

…ater

Architecture dependent: - impl:ice40: GHDL + Yosys + nextpnr-ice40 - impl:ecp5: GHDL + Yosys + nextpnr-ecp5 - impl:pnr: GHDL + Yosys + nextpnr-ice40 + nextpnr-ecp5 - impl:icestorm: impl:ice40 + icestorm - impl:prjtrellis: impl:ecp5 + prjtrellis - impl:latest: impl:pnr + icestorm + prjtrellis

eine · 2021-01-17T05:19:24Z

Hey @eine, sorry for the delay.

No worries at all! The conception of organisation HDL is that all of us are busy and we are already contributing/working on other several open source projects. These repositories are not a priority and we do not need to rush for having features added. The purpose is to provide a robust and reasonably stable set of solutions for the community, which allows us to eventually reduce the maintenance burden in the other repos. That is, if we can provide a container that multiple projects can use, they will use it. If we don't, they won't do CI or they will use their custom solution, which most of the projects are already doing. In other words, this org is a low-pass filter. Sit down, relax, and have fun building these awesome tooling 😉

Should we also make the changes within nextpnr which are the base for some in impl also in a separate PR?
Since the dockerBuild of impl would fail without the necessary images and no dependencies of the workflows.

Yes please. We can split this into as many PRs as you wish. We can keep this one as a reference and handle each relevant modification at a time. As said, no rush. Let's find our rythm. Once you/we get familiar with this codebase, the dance will flow.

(I tried integrating that dependency once, but didn't work as soon as expected and was not patient enough...)

In fact, you cannot make it work. Workflows are independent, thus, executed on independent virtual machines (probably located in different datacenters depending on the day/time). As a result, the images you build in one workflow are not visible to others. Successfully built and tested images need to be pushed to a registry (we use dockerhub), so that later executions of other workflows can use them by name. Yet, you are not allowed to push hdlc/* images to the registry. Therefore, you would need to rename that to something different (a namespace you own). That is not supported currently, as that name is hardcoded in all the dockerfiles and workflows. I suggest to avoid it because it's a lot of manual effort which cannot be upstreamed here. Instead:

You can build images locally. Write a shell script with the sequence of dockerBuild calls in the correct order.
You can add a temporary workflow where you copy the steps from other several workflows. E.g., copy the esteps of workflow nextpnr, along with the ones for impl. The workflow itself won't be upstreamed, but when you push it to your fork, all the images will exist in the same job (virtual machine).

Nonetheless, if you want to actually fork this repo and have your own namespace in an image registry, you are entitled to do so; this is free software. We'd prefer if enhancements were contributed here (as we are doing in these PRs), but any user is implicitly invited to build custom variants.

eine

It looks nice overall! Most of the test scripts, dockerfiles and workflows are good. Just a few fixes and rebasing/splitting.

.github/workflows/impl.yml

eine · 2021-01-17T05:36:51Z

.github/workflows/nextpnr.yml

    - run: dockerBuild nextpnr:ecp5       nextpnr ecp5
    - run: dockerBuild nextpnr:prjtrellis nextpnr prjtrellis
+    - run: dockerBuild pkg:nextpnr-ecp5   nextpnr pkg-ecp5
+    - run: dockerBuild pkg:nextpnr-all    nextpnr pkg-all


Please, rename all to generic and make pkg:nextpnr-generic contain that one only. I think we don't need a pkg:nextpnr-all.

impl.dockerfile

nextpnr.dockerfile

eine · 2021-01-19T06:13:46Z

@se-bi, I think this is all done 🎉, isn't it? Can we close it?

se-bi changed the title ~~Develop~~ Update impl-image, tests and enable tool version strings Dec 20, 2020

eine requested changes Jan 3, 2021

View reviewed changes

eine added the enhancement New feature or request label Jan 3, 2021

umarcor force-pushed the main branch from 4753d5a to db33404 Compare January 3, 2021 16:00

se-bi marked this pull request as draft January 5, 2021 14:16

eine added discussion $R/impl Related to `$R/impl` images and their dependencies $R/pkg Related to `$R/pkg` images and their dependencies labels Jan 11, 2021

se-bi force-pushed the develop branch from 7543c4e to 2f2038f Compare January 14, 2021 23:52

umarcor mentioned this pull request Jan 15, 2021

Add: gowin nextpnr build #10

Draft

se-bi force-pushed the develop branch from 9114b3e to bd941de Compare January 15, 2021 10:59

racerxdl mentioned this pull request Jan 15, 2021

Add Apicula for Gowin FPGA Devices #11

Merged

se-bi force-pushed the develop branch 5 times, most recently from 8a8b069 to 3297403 Compare January 16, 2021 23:24

se-bi force-pushed the develop branch from 3297403 to 80464ec Compare January 17, 2021 00:25

se-bi added 2 commits January 17, 2021 01:29

builds: get source code through git clone to derive version strings l…

5c4abf7

…ater

tests: extend some tests with querying versions of the tools

5216482

se-bi mentioned this pull request Jan 17, 2021

Feature/tool version string #12

Closed

se-bi added 3 commits January 17, 2021 01:45

nextpnr: split build stages; git clone separate from build

bbf3651

nextpnr: Add pkg images

b2cd556

se-bi force-pushed the develop branch from 80464ec to 9949064 Compare January 17, 2021 00:46

eine requested changes Jan 17, 2021

View reviewed changes

se-bi mentioned this pull request Jan 17, 2021

nextpnr: move git clone into earlier build stage #13

Merged

This was referenced Jan 17, 2021

nextpnr: restructure a bit and add pkg images #14

Merged

impl: restructure, add different images with prj tools #15

Merged

se-bi closed this Jan 19, 2021

se-bi deleted the develop branch January 19, 2021 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `impl`-image, tests and enable tool version strings #4

Update `impl`-image, tests and enable tool version strings #4

se-bi commented Dec 12, 2020

eine left a comment

eine Jan 3, 2021

eine Jan 3, 2021

se-bi Jan 16, 2021

eine Jan 17, 2021

se-bi Jan 17, 2021

eine commented Jan 15, 2021

se-bi commented Jan 17, 2021

eine commented Jan 17, 2021 •

edited

Loading

eine left a comment

eine Jan 17, 2021 •

edited

Loading

eine commented Jan 19, 2021 •

edited

Loading

Update impl-image, tests and enable tool version strings #4

Update impl-image, tests and enable tool version strings #4

Conversation

se-bi commented Dec 12, 2020

eine left a comment

Choose a reason for hiding this comment

eine Jan 3, 2021

Choose a reason for hiding this comment

eine Jan 3, 2021

Choose a reason for hiding this comment

se-bi Jan 16, 2021

Choose a reason for hiding this comment

eine Jan 17, 2021

Choose a reason for hiding this comment

se-bi Jan 17, 2021

Choose a reason for hiding this comment

eine commented Jan 15, 2021

se-bi commented Jan 17, 2021

eine commented Jan 17, 2021 • edited Loading

eine left a comment

Choose a reason for hiding this comment

eine Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

eine commented Jan 19, 2021 • edited Loading

Update `impl`-image, tests and enable tool version strings #4

Update `impl`-image, tests and enable tool version strings #4

eine commented Jan 17, 2021 •

edited

Loading

eine Jan 17, 2021 •

edited

Loading

eine commented Jan 19, 2021 •

edited

Loading